-
Notifications
You must be signed in to change notification settings - Fork 16
1268 Vaccination data reporting expired #1269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1269 +/- ##
=======================================
Coverage 97.21% 97.22%
=======================================
Files 156 156
Lines 14674 14709 +35
=======================================
+ Hits 14266 14301 +35
Misses 408 408 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and is consistent in itself. I was wondering why you chose to implement the functionality to set the vacc data to zero in this set_vacc_data() function and not the other one. When you implement it like this it is unclear to me what happens if the simulation interval is only partially contained in the vacc dates. Here https://github.com/SciCompMod/memilio/blob/main/cpp/models/ode_secirts/parameters_io.h#L866 you are already mentioning that for the dates after the end of vacc_data you set the values to 0 but I don't findwhere this is actually done. Also I don't understand why you don't throw a similar warning for the respective min_date, can you explain?
@@ -666,14 +661,14 @@ TEST(TestOdeSECIRVVS, read_data) | |||
|
|||
auto read_result1 = mio::osecirvvs::read_input_data_county(model1, {2020, 12, 01}, {1002}, | |||
std::vector<double>(size_t(num_age_groups), 1.0), 1.0, | |||
TEST_GERMANY_PYDATA_DIR, 10); | |||
TEST_GERMANY_PYDATA_DIR, 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially wanted to write a comment regarding this here :D
The vaccination data for the tests is completely synthetic and starts on 1 October. If we only look 10 days in the future, then is_vaccination_data_available
would return false and no vaccination data would be set. If I set this to 30, then the vaccination data is set because it then returns true ;)
that might be a bit confusing, but it would solve the problem for now. I will adjust the synthetic data in a future pull request to make this more realistic.
Co-authored-by: annawendler <[email protected]>
Because the other function requires the vaccination data. So, if the data is not available, we dont need to read in the vaccination data. |
in the secirvvs model, if data in future is not avialable, we do some linear continuation based on the last entries. If the Simulation In the secirts model, we set all vaccinations to Zero, before and after the vaccination data is avail.
we only set if data is avail. For Dates after, we go in the else loop. FOr all Dates before, we have already initialized it with Zero in
|
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
If need be, add additional information and what the reviewer should look out for in particular:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)